Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix XMLHttpRequest issues #3237

Merged
merged 3 commits into from
Dec 4, 2021
Merged

Conversation

perkinslr
Copy link
Contributor

@perkinslr perkinslr commented Dec 2, 2021

Identify the Bug or Feature request

#3236
#3235

Description of the Change

Changes the decoding for lib:// targets to UTF-8
Changes fetch to an async function, and internally awaits the body of the Request object.
Filters out the pseudo-header :Status from the responseHeaders for the fetch API if the underlying response fails.

Possible Drawbacks

Binary data returned from lib:// targets will be improperly decoded. Binary data should be stored in asset:// instead.

Documentation Notes

Sending body: argument via fetch API now works.
lib:// URIs now return sensible output.

Release Notes

Examples:

  • Sending body: argument via fetch API now works.
  • lib:// URIs now return sensible output.

This change is Reviewable

@@ -56,7 +56,9 @@ public void _getResponseHeaders(JSObject jheaders) {

responseHeaders.forEach(
(key, value) -> {
jheaders.setMember(key, value);
if (!key.equals(":Status")) {
Copy link
Collaborator

@kwvanderlinde kwvanderlinde Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't mentioned explicitly in the linked bug reports, I have a couple questions:

  1. What problems does it cause for this pseudo-header to be present?
  2. Are there other pseudo-headers that should also be filtered out to avoid similar problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! That should have been included in the fetch bug report.
:Status is an intentionally invalid HTTP header which lets the macro writer explicitly set the response code. It is also used internally to get the status code from the request processing out to the wrapper layer. It could use an optional, or similar data type, but exposing those to the macro writers would be a bit of a trick. By specification, invalid HTTP headers must not be returned to the client, so it must be stripped out before the headers are returned. If the response is wrapped in a javascript Response object (as the fetch API does), Response's constructor will throw an exception on invalid headers. Note this is different from undefined / extension headers (valid headers must not start with :).

The problem is the header filtering happens when the response body is received. If there is an exception thrown between when the header default values are set and the response body is received, the body-received function is not called, so the headers do not get filtered. When fetch then tries to handle the failed request, it chokes on the invalid header.

So to sum up:

  1. It breaks the fetch API
  2. No.

That said, I think it might still have a problem outside the fetch API on internal server errors. Probably need to refactor it slightly to always explicitly filter the headers without calling the body-received function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, nope, I can't see why the MTXMLHttpRequest::recv doesn't get called before the headers get retrieved. Especially given that the critical onreadystatechange call which triggers the javascript-side header processing happens after the headers should be handled.

@Phergus Phergus requested a review from kwvanderlinde December 3, 2021 14:12
Copy link
Collaborator

@kwvanderlinde kwvanderlinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @perkinslr)

Copy link
Collaborator

@kwvanderlinde kwvanderlinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @perkinslr)

@Phergus Phergus merged commit a73bf1c into RPTools:develop Dec 4, 2021
Phergus added a commit that referenced this pull request Dec 4, 2021
Phergus added a commit that referenced this pull request Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants